Parameter parsing fixes #309
Conversation
|
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DBAL-501 We use Jira to track the state of pull requests and the versions they got |
|
|
||
| if ($isParam) { | ||
| throw SQLParserUtilsException::missingParam($paramName); | ||
| } else { |
There was a problem hiding this comment.
no need for else as the if throws an exception
There was a problem hiding this comment.
Mutual exclusiveness is better communicated that way. That's why there is an else.
There was a problem hiding this comment.
but it does not follow our coding standards
There was a problem hiding this comment.
|
using this branch I was able to successfully run the Jackalope Doctrine DBAL test suite with MySQL. Note for SQLite I wasnt able to run the test suite since it seems like drop FK statements are no longer ignored which makes the DB rest (https://github.com/jackalope/jackalope-doctrine-dbal/blob/master/src/Jackalope/Transport/DoctrineDBAL/RepositorySchema.php#L120) in our test suite fail: but that is another story :-/ |
| } | ||
|
|
||
| // Hash keys can be prefixed with a colon for compatibility | ||
| if (isset($paramsOrTypes[':' . $paramName])) { |
There was a problem hiding this comment.
wait, so PDO supports $stmt->bindParam(':foo', $val); instead of $stmt->bindParam('foo', $val);
There was a problem hiding this comment.
Yep, that was the root cause of the compatibility break with the Jackalope DBAL component.
There was a problem hiding this comment.
I didn't know that, hence that code was never tested. :)
|
@beberlei could you advise on where to go from here? |
|
@lstrojny thinking about the SRP violation, a solution would be three methods,l but not sure if this is necessary here. Pondering with the problem... :) |
|
Sure, three methods would work |
Fix for #301 has been reverted to address http://www.doctrine-project.org/jira/browse/DBAL-496. This fix addresses both issues in a consistent manner and ads a few more tests.